fix(account): preserve then rebuild an unreadable account store - #261
Conversation
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed at 5f20182, sitting on top of #253's 4b9f41d. Gates on that head: typecheck clean across all three projects, lint 0 errors and the same 15 warnings, format check clean, test:coverage green at 130 files, 1,583 passed, 2 skipped, statements 92.47, branches 89.63, functions 91.5, lines 94.01, all above the floors.
The design call is right and the reasoning behind it holds. Sessions in a file that stopped decrypting are gone whether or not you write over them, so refusing the write recovers nothing and costs the player a launcher that can never save an account again, with no way to clear the dead file from inside the app. Copying aside and rebuilding preserves the same bytes a refusal would while letting the player's own deliberate login succeed, and the narrow throw when the copy itself fails is in the right place: that really is the only case where proceeding destroys rather than merely fails to read.
Locked keyring versus corrupted file: the code gets it right, nothing pins it. Blocking.
This is the distinction the whole fix turns on, and it is handled well. readStore returns early on a failed assertSecureStorage with unreadable: false, and deliberately does not cache, so a keyring that unlocks later still reaches the real file. writeAccounts asserts the same thing before it could overwrite anything, so a locked keyring cannot reach the rebuild at all. Exactly the split I asked about.
No test holds it there. Flipping that early return to unreadable: true leaves all 130 files and 1,583 tests green.
What that mutation does in the field is worse than it first looks. A player with a perfectly good store and a locked keyring logs in, preserveUnreadableStore copies their intact secrets file to account-secrets.unreadable.bak.json, then writeAccounts throws and the login fails anyway. They are left with a second copy of their encrypted store on disk, and because the snapshot uses overwrite: false, that stale copy permanently occupies the slot a genuine corruption event would have needed later. The recovery path this PR builds gets poisoned by the first locked-keyring login.
One test closes it: an intact v2 store on disk, encryptionAvailable false, assert saveAccountSecrets rejects, assert account-secrets.unreadable.bak.json was never created, and assert the store file is byte-for-byte what it was. The two existing "refuses to write when..." cases are close but both start from no store file at all, so neither can catch this.
The rest checks out
I ran your own mutation to confirm the harness. Reverting saveAccountSecrets to the unconditional writeAccounts turns exactly 5 of the new cases red, matching what you reported. The eight cases are well chosen, and "treats a store whose entries it merely dropped as readable, not unreadable" is the one that keeps this from firing on the ordinary partial-loss case, which is the failure mode I would have expected a fix like this to introduce.
Secrets hygiene is clean. Nothing decrypted reaches a log: the rebuild warning in accountHandlers.ts and the one in adoptRefreshedSession both say that a rebuild happened, never what was in it. storeRebuilt crosses the bridge as a boolean and session-store-unreadable as a status string, neither carrying anything out of the store. preserveUnreadableStore uses fse.copy, which chmods the destination to the source's mode, so the 0600 on account-secrets.json follows the snapshot. Worth an assertion in the test above while you are in there, since that property comes from fs-extra rather than from this file and could change under you without anything here noticing.
Keeping session-store-unreadable off the success union while storeRebuilt rides on it is the right split. The account never reaches config when nothing could be saved, which avoids a config entry with no secrets behind it, and SessionButton returning early on that status is consistent with it.
One non-blocking thought on first-snapshot-wins
The argument in the comment, that the earliest unreadable file is the one most likely to still hold every account ever saved, runs the other way about as often. Corruption hits when one account is saved, the snapshot is taken, the store rebuilds and grows to four accounts, corruption hits again, and the second snapshot is skipped because the first is there. The four-account bytes are the ones destroyed. For genuinely undecryptable bytes that is academic either way, but the version-mismatch case is not: a store written by a newer build really is recoverable by that build, and that is where losing the later snapshot costs something real.
Nothing to change now. If you keep the current policy, it is worth saying in the comment that the snapshot is deliberately one-shot and the trade is accepted, rather than resting it on a "most likely" that does not always hold.
Related, for a follow-up rather than here: nothing in the app ever surfaces or clears account-secrets.unreadable.bak.json, so it sits there indefinitely and silently blocks the next snapshot. The toast is honest about the other accounts needing to log in again, but there is no path back to the preserved bytes and no way to free the slot.
Merge order
These two interlock, so worth being explicit.
#253 on its own still overwrites a v1 store that decrypts wrong with no snapshot, because adoptLegacySingleAccountSecrets only takes its backup after a successful decrypt. This PR is what closes that: readStore classifies a v1 file as unreadable on the version check, so the next login preserves it rather than flattening it. It also ends #253's re-key retry cleanly, since the rebuild leaves a v2 file and adoptLegacySingleAccountSecrets no-ops from then on. There is no conflict between them, because #253 carries no competing rebuild logic, only the comment in writeAccounts acknowledging the hole, which this PR replaces outright.
So #253 first, then this. I would prefer merging this into feat/issue-238-multi-account and taking the pair to dev as one, so dev never carries a build where an unreadable store gets overwritten without a snapshot. Merging #253 to dev and retargeting this also works. Do not land this on dev ahead of #253.
Requesting changes for the locked-keyring test.
removeAccountSecrets returned true when the account was absent, including when it was absent only because the store could not be read (a locked keyring, or bytes that stopped decrypting). The renderer took that as success, dropped the account from config, and told the player it was gone while its session sat on disk under a uid nothing named any more. It now returns readStore().unreadable => false in that case, so the renderer keeps the account and shows the store error instead. No rebuild: only saveAccountSecrets does that. Adds the locked-keyring regression test #261's review asked for: an intact v2 store plus an unavailable keyring must leave the file byte-for-byte, at its original mode, and must not create the one-shot unreadable snapshot. Reworks the preserveUnreadableStore comment to state the first-snapshot-wins trade is deliberate and accepted rather than resting it on a 'most likely'. PR #261 review.
5f20182 to
b4accd9
Compare
|
Rebased onto The locked-keyring test is in. The On the first-snapshot-wins note, I kept the policy and rewrote the comment to own the trade rather than rest it on a "most likely": a second corruption event after the store has rebuilt and grown can hold more than the kept snapshot, and for a version-mismatch file that is a real loss, but a single predictable recovery file beats an unbounded pile of them, and nothing surfaces or clears these yet anyway. That last part (no path back to the preserved bytes, no way to free the slot) is the follow-up you flagged; it wants its own issue off #259 rather than more code here. The 0600-follows-the-snapshot property you asked about is now asserted in the locked-keyring test above ( Gates on the rebased head, local, Node 24.15.0, WSL: Still a draft, still stacked: #253 first, then this retargets to |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Approving at b4accd9.
The locked-keyring test does what it needed to do. Flipping the readStore early return to unreadable: true turns "does not snapshot or touch an intact store when only the keyring is locked" red and nothing else, 1 failed out of 1,631. That mutation survived every test last round, and it is the one that mattered most here: a locked keyring is a transient, ordinary condition, and treating it as corruption would copy a perfectly intact store into the single overwrite: false snapshot slot and then fail the login anyway, burning the one recovery file before a real corruption ever needs it. The test asserts the three things that actually pin it, no snapshot file, the store byte-for-byte, and the mode unchanged, so a partial rewrite that preserves only the bytes still gets caught.
Your own mutation still holds too. Collapsing saveAccountSecrets back to an unconditional writeAccounts fails 5 of the preserve tests: the undecryptable case, the not-JSON case, the unknown-version case, the pre-migration backup collision and the copy-failure refusal, 5 out of 1,631.
b4accd9 also closes the removeAccountSecrets note I left on #253, and it closes it the right way. Returning !store.unreadable when the delete finds nothing means a locked keyring or a file that stopped decrypting reports failure instead of telling the player an account was removed while its session sits on disk under a uid nothing names any more, and it gets there without giving removal a rebuild path of its own. The reworked preserveUnreadableStore comment reads better than the old one as well, since it states the first-snapshot-wins trade as a decision with a known cost rather than resting it on a likelihood.
Gates on this head: typecheck clean on all three projects, lint:ci at 0 errors with the same 15 warnings, format:check clean, and test:coverage at 137 files, 1,629 passed and 2 skipped, with statements 92.6, branches 89.84, functions 92.03 and lines 94.05, all above the floors.
Merging #253 first, then retargeting this to dev and taking it straight after, which keeps the ordering these reviews have been assuming throughout.
Fixes #259. readAccounts caught every read failure the same way, absent file or undecryptable one, and cached an empty map either way. saveAccountSecrets then wrote that empty map back over whatever was on disk. At single-account scale that cost the one account the old store held; since multi-account (#238) it costs every saved account's session in one shot, the instant any player logs back in. The sessions in an unreadable file are already gone the moment it stops decrypting: refusing the write recovers none of them, it only leaves the launcher unable to save any account ever again, with nothing in the app to clear the dead file. So this preserves the unreadable bytes once (preserveUnreadableStore, mirroring the existing pre-migration backup convention but under its own path so the two events can never collide) and rebuilds the store around the account logging in now, matching this codebase's own adoptRefreshedSession precedent: a storage problem must not block the player's own deliberate action. The one exception is when the bytes cannot even be copied aside (a permissions problem, most likely); proceeding there really would destroy something, so it throws AccountStoreUnreadableError instead. readStore now distinguishes an absent file (the ordinary no-accounts-yet case, unchanged) from a present-but-unreadable one via a new `unreadable` flag, which only saveAccountSecrets reads; every other caller still just wants the map. saveAccountSecrets returns a typed AccountSaveOutcome ("saved" or "saved-after-rebuild") instead of void. Wired into LOGIN: a rebuild flags the success result with `storeRebuilt: true` rather than a status of its own, since the login did succeed and a separate status would make every `status === "success"` check silently drop the account. The narrow copy-failure case gets its own AccountLoginResult status, `session-store-unreadable`, the same honesty this file already gives `unexpected-response`: the credentials were fine, only the local save failed. SessionButton shows a warning toast on a rebuild and a distinct error on the unreadable case, neither collapsing into "invalid email or password". adoptRefreshedSession (gameHandlers.ts) logs the rebuild but needed no behavior change, since it already treats a storage failure as non-fatal to the launch. 8 new tests in accountStore.test.ts cover the rebuild path: bytes preserved byte-for-byte, a dropped-entry file still treated as readable (not rebuilt), the first snapshot kept across repeated corruption, no collision with the pre-migration backup, and the copy-failure refusal. 2 new tests in accountHandlers.test.ts, 1 in accountLoginOutcome.test.ts, and a new sessionButtonStoreRebuilt.test.tsx cover the wire status and the UI. Verified the harness catches a real regression: reverting saveAccountSecrets to the old always-overwrite version turned 5 of the new accountStore tests red, then reverted cleanly.
removeAccountSecrets returned true when the account was absent, including when it was absent only because the store could not be read (a locked keyring, or bytes that stopped decrypting). The renderer took that as success, dropped the account from config, and told the player it was gone while its session sat on disk under a uid nothing named any more. It now returns readStore().unreadable => false in that case, so the renderer keeps the account and shows the store error instead. No rebuild: only saveAccountSecrets does that. Adds the locked-keyring regression test #261's review asked for: an intact v2 store plus an unavailable keyring must leave the file byte-for-byte, at its original mode, and must not create the one-shot unreadable snapshot. Reworks the preserveUnreadableStore comment to state the first-snapshot-wins trade is deliberate and accepted rather than resting it on a 'most likely'. PR #261 review.
b4accd9 to
1f2dd31
Compare
|
Note on the commit that landed after my approval, since it is mine and not the author's. I approved at So I rebased the two commits onto the new dev and force-pushed, Re-verified on One thing worth recording that has nothing to do with this PR: on one of those coverage runs Approval stands. Merging this now. |
|
Taking this out of draft, since the condition the description set for that is now met: "This should be retargeted to |
2 participants
Summary
An undecryptable or malformed account-secrets store used to be indistinguishable from an absent one:
readAccountscaught every failure the same way and cached an empty map either way, so the next login silently overwrote the file, discarding every other saved account's session, not just the one being saved. This makessaveAccountSecretspreserve the unreadable bytes once, then rebuild the store around the login that is actually happening, and tells the caller which one occurred.Observed problem
Filed as #259 after review of PR #253: "An undecryptable store now costs every saved account its session rather than the one the old store held.
readAccountscaches an empty map on any failure and the nextsaveAccountSecretswrites that map back over the file. Your comment inwriteAccountssays exactly this and I think the call is right given the backup, but it is worth a follow-up issue rather than only a comment, because the blast radius genuinely grew."At single-account scale, an undecryptable store cost the one account it held. Since multi-account, the same code path costs every saved account on the device in one shot, triggered by any single player logging back in.
Design decision
The issue itself named the crux: "letting a re-login for one account fail outright when a housemate's store is unreadable is its own UX question." Two options existed, refuse the write or preserve and proceed. This takes preserve and proceed, for one reason that settles it: the sessions in an unreadable file are already lost the instant it stops decrypting. Refusing the write does not bring any of them back, it only leaves the launcher permanently unable to save any account at all, with nothing in the app to clear the dead file for it. A copy-aside preserves exactly the same bytes a refusal would, while still letting the player's own deliberate login succeed, matching this codebase's own precedent in
adoptRefreshedSession(gameHandlers.ts): a storage problem must not block the action the player actually asked for.The one place refusal is correct is narrower: when the unreadable file cannot even be copied aside (most likely a permissions problem). There, proceeding would destroy bytes rather than merely fail to read them, so that case throws instead of writing anything.
Fix
src/ipc/accountStore.ts:readStore()replaces the oldreadAccounts()caching logic, returning{ accounts, unreadable }. A genuinely absent file (ENOENT) is notunreadable, it is the ordinary no-accounts-yet case and behaves exactly as before. Anything else that keeps the read from succeeding (wrong version, bad JSON, decrypt failure, a decrypted payload with noaccountsarray) setsunreadable: true. A file holding one entryparseStoredSecretsByIditself drops, beside other good entries, is not corruption: that is still a store worth writing to.preserveUnreadableStore()copies the current store file to a new path,account-secrets.unreadable.bak.json, withoverwrite: falseso the first snapshot survives repeated corruption events. Deliberately a separate file from the existingaccount-secrets.pre-migration.bak.json: those are two different events (an old-format file being upgraded, versus a current-format file that stopped decrypting), and sharing one path would let whichever happens second silently erase the other's snapshot.AccountStoreUnreadableErroris thrown when the copy itself fails, the one case where proceeding would destroy something.saveAccountSecretsnow returns a typedAccountSaveOutcome,"saved"or"saved-after-rebuild", instead ofvoid.src/global.d.ts:AccountLoginResultgets a new status,session-store-unreadable(the credentials were accepted, but nothing could be saved), and an optionalstoreRebuiltflag onsuccess. Not a separate success status: the login did succeed, and a separate status would make everystatus === "success"check silently drop the account.src/ipc/handlers/accountHandlers.ts'sLOGINhandler: a rebuild logs a warning and setsstoreRebuilt: trueon the success result; anAccountStoreUnreadableErrorresolvessession-store-unreadableinstead of falling into the generic "Login failed" throw, the same honesty this file already givesunexpected-responsefor a different failure: the credentials were never actually the problem.src/renderer/src/components/ui/SessionButton.tsx: a rebuild shows a warning toast naming what happened;session-store-unreadableshows its own error, distinct from "invalid email or password". Two newen-US.jsonstrings.src/ipc/handlers/gameHandlers.ts'sadoptRefreshedSessionlogs the rebuild too, but needed no behavior change: it already treats a storage failure as non-fatal to the launch, matching the design principle this fix leans on.Regression proof
tests/ipc/accountStore.test.tsgets a newdescribeblock, 8 cases: bytes preserved byte-for-byte across an undecryptable file, a non-JSON file, and a future-version file; a file holding only a dropped entry treated as readable, not rebuilt; the first snapshot kept across a second corruption event; no collision with the pre-migration backup; and the copy-failure refusal (Linux-only, skipped as root).tests/ipc/accountHandlers.test.tsgets 2 new cases for the wire status and thestoreRebuiltflag.tests/ipc/accountLoginOutcome.test.tsgets one forsessionStoreUnreadableResult. A newtests/renderer-dom/sessionButtonStoreRebuilt.test.tsx(3 cases) covers the toasts end to end.tests/ipc/gameHandlers.test.tsandtests/ipc/configManager.test.tsneeded theirsaveAccountSecretsmocks updated to resolve the new return type.Verified the harness catches a real regression: reverted
saveAccountSecretsto the old always-overwrite version, rantests/ipc/accountStore.test.ts, and 5 of the 8 new cases went red exactly as expected, then reverted and confirmed the file diff was clean.Testing
npm run typecheck: passes, all three projects.npm run lint:ci: 0 errors, 15 pre-existing warnings.npm run format:check: passes.npm run test:coverage: 130 files, 1,583 passed, 2 skipped. Coverage 92.47% statements, 89.63% branches, 91.5% functions, 94.01% lines, all at or above thevitest.config.tsfloor.npm run build:unpack: passes on Linux x64.Base branch
This PR targets
feat/issue-238-multi-account(PR #253), notdev, for the same reason PR #260 targetsfix/issue-248-vsl-link-palette: the multi-account account store (readAccounts,writeAccounts,saveAccountSecretskeyed byplayerUid) this fix is about does not exist ondevyet, only on #253's branch. This should be retargeted todev(or rebased and reopened) once #253 merges; until then it is a stack, not an independent change.Limitations
Not verified against a real OS keychain, same limitation as #253 itself: the encryption layer is faked in tests, and the
preserveUnreadableStorecopy-failure case was only exercised via a chmod'd directory on Linux, not a real permissions failure on Windows or macOS.Related issues
Fixes #259.